The skipped status must be one the harness owns (#447) - #455
Merged
jdatcmd merged 1 commit intoAug 6, 2026
Conversation
) commandprompt#448 used exit 2 for "ran no checks". That was wrong. 2 is a status suites already produce for unrelated reasons: bash exits 2 on a parse error in the suite file, and smoke, phase2 through phase6 and audit run under set -euo pipefail, so they abort with whatever status the failing command returned. A dead postmaster or a typo therefore became "ran no checks", every runner printed SKIP, and the major reported PASS. That is the lie commandprompt#447 was opened to remove, relocated one layer down and shipped by the fix for it. Now 66, which bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make do not produce. That cannot be made collision-proof, because set -e propagates any status, so the runners require the SKIPPED line in the log as well. Two independent signals. Also from the same review: - native_parquet_streaming's sparse-file gate still called bare pgc_summary. The pyarrow gate above it was converted and this one was missed, so a low-space filesystem took the 1GB-palloc ceiling and the 1600MB-hole guard out of the run while the major passed. - run_san's rc branch was justified by the pyarrow suites, which cannot reach it: a missing dependency FAILS, so pgc_skip exits 1. Comment corrected to describe what the branch actually catches. - run_san reported PASSED with 22 of 23 suites skipped, because the guard fired only when ALL skipped. Any skip now makes the gate incomplete. - run_coverage got the skip bucket and not the ran-nothing guard, so its only verdict was 'nothing failed' -- which a box where every suite aborts satisfies. - pg19_vacuum_options and native_repack read an unanswerable version probe as 'not PG19' via ${srv:-0}. A dead cluster now fails. The failure is raised only when it occurs, because a check that always ran would put PGC_CHECKS at 1 and destroy the very skip it guards on older majors.
jdatcmd
approved these changes
Aug 6, 2026
jdatcmd
left a comment
jdatcmd
left a comment
Collaborator
There was a problem hiding this comment.
Approving. I verified the central claim rather than taking it, because I approved #450 and this is a defect in code I passed.
On origin/main, right now:
run_all_versions.sh:459 elif [ "$_rc" = 2 ]; then <- classified as skip
run_all_versions.sh:609 elif [ "$_exrc" = 2 ]; then <- same, extended runner
$ printf 'if [ 1\n' > bad.sh && bash bad.sh; echo $?
2
psql exits 2 on a bad connection, and smoke, phase2-phase6 and audit run under set -euo pipefail. So a suite with a syntax error, or one that aborts because the postmaster is gone, is reported as SKIP and the major reports PASS. That is #447's lie relocated one layer down by the fix for it, exactly as you describe.
The two-signal design is the part I would have got wrong. 66 is unreachable for bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make, but set -e propagates whatever an aborting command returns, so no single status can be made collision-proof by choosing a rarer number. Requiring the SKIPPED (ran no checks) marker in the log alongside it is what closes that, and saying so in the PR rather than claiming 66 is safe is the right framing.
The five follow-ons are the same shape as the thing they fix, and native_parquet_streaming's bare pgc_summary is the worst of them: a low-space or non-sparse filesystem silently removed the 1GB-palloc ceiling and the 1600MB-hole guard while the major reported PASS. The dead exit 0 being the tell is a good detail to have written down.
run_san printing SANITIZER GATE PASSED with 22 of 23 suites skipped is the one I would have shipped myself — a guard that fires only when all skipped reads as correct until you write out what "any" means.
Two notes, neither blocking:
run_coveragesettingPGC_SKIP_TIMING=1is right, and it now depends oncheck_ratio_timingfrom #444 being present to actually drop ratio checks rather than fail them. Worth checking the merge order between these two.${srv:-0}on the version probe: agreed that raising only when it occurs is what preserves the skip on older majors. That reasoning deserves to stay in the comment, because the obvious "simplification" is a check that always runs, and it would destroy the skip it guards.
ChronicallyJD
pushed a commit
to ChronicallyJD/pgcolumnar
that referenced
this pull request
Aug 7, 2026
…cy (commandprompt#414) CI went red on PG17 the moment this suite was registered: FAIL pgcolumnar.analyze() needs pg_restore_attribute_stats (PG18+); this server is 17 FAIL PG17 (124 ran, 7 skipped) The gate used pgc_skip, which is the wrong instrument. pgc_skip is for a missing DEPENDENCY -- pyarrow, nm -- which is an environment defect, so it fails by default and has to be waived deliberately, because somebody should install the thing. A major that does not ship pg_restore_attribute_stats is not a defect anyone can fix: 15 to 17 genuinely lack it, the same way 15 lacks WITHOUT OVERLAPS. Failing there is a red nobody can act on, which is the kind that teaches readers to discount red. So it reports SKIP and runs no checks, which pgc_summary turns into PGC_EXIT_SKIPPED and the matrix records as SKIP (commandprompt#447, commandprompt#455). Same shape as pg19_vacuum_options on anything below 19; verified both exit 66 on PG17. The major is asserted before the comparison. An unreadable version must not be mistaken for an old one, or a broken environment reports SKIP and reads as "this major does not support it". Gated on a FRESH tree per major. Two false reds while checking this were mine, not the code: the first was leftover postmasters holding the port band, and the second was building PG17 and PG18 in one tree, so PG18 linked stale objects and the postmaster would not load the library. Both present as "no cluster of our own after 8 attempts", which looks nothing like its cause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2DvnWDM7g27ubDCQdXhky
ChronicallyJD
pushed a commit
to ChronicallyJD/pgcolumnar
that referenced
this pull request
Aug 7, 2026
Review catch, and it is this script's own principle in the one place the script did not apply it. Both SKIP paths and the pid-collision FAIL returned from profile_shape without recording anything, and nothing after the loop inspected anything. A run in which EVERY shape collided printed its FAIL lines, then "== profile complete ==", and exited 0: the summary claiming success while nothing had been measured. Two counters rather than one, which is the lesson from commandprompt#447 and commandprompt#455 -- a single status value cannot be made collision-proof, and commandprompt#455 had to fix commandprompt#447 again a layer down for exactly that reason. A defect and an underpowered run are different things and are now reported and exited differently: 0 every requested shape produced a usable profile 1 a shape FAILED: pid collision, no backend appeared, or a shape name that does not exist 2 nothing failed, but a shape produced too few samples to attribute The too-few-samples case is deliberately not called FAIL. It is a legitimate "raise PROFILE_SECS" condition rather than a defect, so it is counted as not-success and named, without being reported as a fault in the code under test. An unknown shape now fails too. It previously profiled nothing and exited 0, which is the same defect with an easier cause: a typo in PROFILE_SHAPES. Proved by removal, not asserted: PROFILE_SHAPES=bogus -> "== profile FAILED == no usable profile for: bogus" exit 1 PROFILE_SHAPES=decode -> "== profile complete ==" exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRQYekvivA4RLDnndhanHK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#448 used exit 2 for "ran no checks". That was wrong, and it is my defect.
2 is a status suites already produce for unrelated reasons. bash exits 2 on a parse error in
the suite file, and
smoke,phase2-phase6andauditrun underset -euo pipefail, so theyabort with whatever status the failing command returned. A dead postmaster or a typo therefore
became "ran no checks", every runner printed SKIP, and the major reported PASS.
That is the lie #447 was opened to remove, relocated one layer down and shipped by the fix for it.
Now 66, which bash (1, 2, 126, 127, 128+n), psql (1, 2, 3) and make do not produce. It cannot
be made collision-proof, because
set -epropagates any status an aborting command returns, sothe runners also require the
SKIPPED (ran no checks)line in the log. Two independentsignals, because one was not enough.
Four more from the same review, all in merged code
native_parquet_streaming's sparse-file gate still called barepgc_summary. The pyarrowgate thirty lines above it was converted and this one was missed, so a low-space or non-sparse
filesystem took the 1GB-palloc ceiling and the 1600MB-hole guard out of the run while the major
reported PASS. The dead
exit 0after it was the tell.run_san's skip branch was justified by the pyarrow suites, which cannot reach it. A missingdependency FAILS, so
pgc_skipexits 1. The comment described a case the code could not catch.run_sanprintedSANITIZER GATE PASSEDwith 22 of 23 suites skipped, because the guardfired only when all skipped. Any skip now makes the gate incomplete.
run_coveragegot the skip bucket but not the ran-nothing guard, so its only verdict was"nothing failed" — which a box where every suite aborts satisfies perfectly. It now also sets
PGC_SKIP_TIMING=1, because a--coveragebuild's wall clock means nothing and this runnerdiscovers every
test/*.sh.pg19_vacuum_optionsandnative_repackread an unanswerable version probe as "not PG19"via
${srv:-0}. A dead cluster now fails. The failure is raised only when it occurs: a checkthat always ran would put
PGC_CHECKSat 1 and destroy the very skip it guards on older majors.Found by a high-effort review of
test/, then verified on the bench.